-
-
Notifications
You must be signed in to change notification settings - Fork 153
GH1383 Add support for creating Series/Index with dtype='category' #1391
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Thank you for the proposal, it looks good to me.
|
|
I tried uncommenting the lines and it is raising many errors, I have not thing examples of Series[category] in the code yet. |
I think those have been commented out since they were first created. I think they can be uncommented, but they should be using |
cmp0xff
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Could you merge
maininto your branch? Resolving conflicts should be easy. - If you haven't, could you try the idea in #1391 (comment), namely changing to
Series[CategoricalDtype]? - Otherwise it looks good to me, now that the idea of having
Series[CategoricalDtype]has also been approved.
|
|
cmp0xff
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s.astype("dictionary[pyarrow]")isUnknown, not sure if we have the correct overload for it. Also when I try to do it at run time I get aTypeErrorso wondering what is the goal of having that only for type checking and not run time. Open to suggestions
I see, those commented-out tests do not make sense to me, either.
One small thing, otherwise good to go.
tests/series/test_properties.py
Outdated
|
|
||
| def test_array_property() -> None: | ||
| """Test that Series.array returns ExtensionArray and its subclasses""" | ||
| # casting due to pandas-dev/pandas-stubs#1383 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove thie comment
If they don't work at runtime, then let's just delete them and not worry about it. |
cmp0xff
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good to me now, but you have not yet requested me to review again. Would you agree to merge @loicdiridollou ?
|
Yes good to merge (I often forgot to rerequest sorry) |
|
Thank you @loicdiridollou! I am getting familiar with the workflow, hopefully I'll be able to do the review better in the future. |
Index([1], dtype="category")andSeries([1], dtype="category")are mistyped #1383assert_type()to assert the type of any return value